Skip to content

Stop coalescing NULL to empty string in key provider functions #367

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 5, 2025

Conversation

jeltz
Copy link
Collaborator

@jeltz jeltz commented May 26, 2025

Since we can pass NULL values in the functions which takes JSON it makes no sense to not do so in the functions which take text. By doing this we only risk not excercising some code paths.

@jeltz jeltz requested review from dutow and dAdAbird as code owners May 26, 2025 13:59
@AndersAstrand
Copy link
Collaborator

AndersAstrand commented May 26, 2025

Does anyone involved when these were added remember why it was done? I'm all for removing them unless we have really good reasons to have them, as they might hide bugs

Copy link
Collaborator

@dutow dutow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also add some testcases that uses nulls with this?

If I remember correctly the original json parser accepted empty strings, but reported an error on nulls, which wasn't ideal for optional parameters such as a CA file.

@AndersAstrand
Copy link
Collaborator

Can we also add some testcases that uses nulls with this?

If I remember correctly the original json parser accepted empty strings, but reported an error on nulls, which wasn't ideal for optional parameters such as a CA file.

If there are bugs in there that the coalesce()s hide we should definitly merge this pull request asap so they can be exposed.

Since we can pass NULL values in the functions which takes JSON it makes
no sense to not do so in the functions which take text. By doing this we
only risk not excercising some code paths.
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.35%. Comparing base (d37fa69) to head (fbd9da4).

❌ Your project status has failed because the head coverage (85.35%) is below the target coverage (90.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files
@@                  Coverage Diff                  @@
##           TDE_REL_17_STABLE     #367      +/-   ##
=====================================================
+ Coverage              85.20%   85.35%   +0.15%     
=====================================================
  Files                     22       22              
  Lines                   2534     2533       -1     
  Branches                 384      383       -1     
=====================================================
+ Hits                    2159     2162       +3     
+ Misses                   301      297       -4     
  Partials                  74       74              
Components Coverage Δ
access 84.20% <ø> (ø)
catalog 88.44% <ø> (+0.48%) ⬆️
common 91.80% <ø> (ø)
encryption 72.56% <ø> (ø)
keyring 71.94% <ø> (-0.07%) ⬇️
src 91.40% <ø> (ø)
smgr 97.41% <ø> (ø)
transam ∅ <ø> (∅)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jeltz jeltz merged commit 4173798 into percona:TDE_REL_17_STABLE Jun 5, 2025
16 checks passed
@jeltz jeltz deleted the tde/coalesce branch June 6, 2025 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants